ci: narrow the GPU serve matrix with a serve paths-filter (EAI-7746) - #156

Automated review pass over the diff against main (CI workflow + harness + features reviewed in full, plus surrounding context). No code changed. Findings prioritized; nothing here is a hard blocker.

Findings

1. tests/e2e-cucumber/README.md:98-106 — tag table is stale (please fix in this PR).
The table is the documented contract for the tag vocabulary (@id, @requires-gpu, @requires-engine, @requires-os, @serve-timeout, @nightly, @lifecycle). @canary and @serves-on-gpu are missing. Failure scenario: a contributor adds a scenario that real-serves on a GPU host without @requires-gpu, has no way to learn @serves-on-gpu exists, omits it — and it launches an extra real vLLM serve on every PR canary run, re-inflating exactly the cost this PR removes. Same file: line 91-94 ("Each CI job runs the whole suite … no -t filter") and the job table at 125-136 are now inaccurate — e2e-gpu on a PR runs one scenario, and the two Strix lanes don't run on PRs at all.

2. tests/e2e-cucumber/src/expectation.rs:373-381 + features/model_serving.feature:84 — nothing enforces "exactly one @canary".
Verified exactly one scenario carries it today, and that no code, test, or CI step checks cardinality. Failure scenario: a later refactor of scenario 6b drops or renames the tag. E2E_PR_CANARY=1 then skips every GPU-serving scenario with nothing left to serve — the lane goes green in ~2 minutes, platform.json is well-formed, reconciliation is clean, and the pre-merge serve smoke has silently stopped testing anything real, with no signal anywhere. A unit test that parses the real .feature files and asserts a single @canary would close this cheaply.

3. .github/workflows/ci.yml:962-979, 1122-1139 (with continue-on-error at 777/979/1139) — "the full matrix runs in the merge queue" implies protection that isn't there.
All three GPU jobs are continue-on-error: true, including under merge_group. So a real Strix (lemonade / Windows) serve regression now: skips on the PR, runs in the merge queue, fails without blocking, and lands on main. Previously it at least showed up as a red non-blocking check on the PR, in front of the author. Effective detection moves to nightly, a day later, decoupled from the causing PR. That may well be the trade you want given serial hardware — but the comments and PR body read as if the merge queue is a backstop, and it isn't a gating one. Suggest either saying so plainly in the comment, or dropping continue-on-error on the merge_group path.

4. .github/workflows/ci.yml:131-157 — the crates/rocm-dash-* exclusion rests on an unverified runtime-isolation assumption.
apps/rocm/Cargo.toml:20-31 shows rocm depends directly on rocm-dash-daemon / rocm-dash-tui (transitively rocm-dash-core, rocm-dash-collectors), none of which are in the serve filter. Compile risk is genuinely covered (heavy/rust still match **/*.rs), so this is only about shared process state in the single rocm binary — global tracing/config/signal init. Failure scenario: a dash-only PR perturbs shared init, the GPU matrix skips on the PR, and it's caught one stage later when merge_group forces serve=true. Low severity, and the exclusion itself is structurally sound: root-only Cargo.toml is the right call, and I confirmed no other glob (scripts/**, xtask/**, **/*.feature) re-includes the dash crates. I did not trace apps/rocm verb dispatch to confirm isolation.

5. tests/e2e-cucumber/src/expectation.rs:373-381 — gate ordering yields a misleading skip reason.
The canary gate sits before the requires_gpu && !cap.has_amd_gpu check, so a @requires-gpu scenario on a GPU-less host in canary mode would report "skipped in PR canary mode" rather than "requires an AMD GPU". Verified unreachable today (E2E_PR_CANARY is set only on the MI300X lane) and harmless if reached — crates/e2e-report reconciles on the expected label and never parses reason. Swapping the two gates is a one-line defensive fix.

6. tests/e2e-cucumber/src/expectation.rs:356-364 — resolve() is now three trailing bools deep (include_nightly, include_lifecycle, canary_mode), all same-typed and positional, with ~20 call sites passing false, false, false. A future swap of two of them compiles clean and silently mis-resolves. Cross-PR note: #155, #156 and #157 are each independently growing this parameter list — one shared RunMode-style struct beats three separate fixes, so it's probably worth agreeing where that lands rather than each PR adding a bool.

Also cross-PR: the README.md tag table is stale in both this PR and #157 — whichever lands second should be the one to fix it.

Verified clean

• @serves-on-gpu tagging is complete. Traced every non-@requires-gpu scenario across all 9 feature files into its step definitions. chat.feature 5/6 (serving_steps.rs:555-576, branches on has_amd_gpu) and model_serving.feature 1/2 (serving_steps.rs:590-604, unconditional real rocm serve) are the only real-servers, and all four are tagged. No over-tagging either. This was the highest-risk item in the change and it's closed.
• Serve dependency closure is covered: engines/** catches rocm-engine-lemonade/-vllm, apps/rocmd/** catches rocmd, plus rocm-core and rocm-engine-protocol. Only the dash crates fall outside (see Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex #4).
• Off-PR forcing (ci.yml:87 plus the all/forced step) genuinely resolves serve to true on merge_group/push/dispatch.
• serve really is not a subset of heavy, so the || serve report gate is load-bearing, not cosmetic: crates/rocm-core/src/model_catalog.json matches serve but not heavy. Traced that path — build-and-test still reports success (its job-level if is trigger-only; the steps are heavy-gated), so e2e-gpu runs and produces the artifact the report job needs.
• Branch-protection reasoning holds: the required contexts on main include "E2E tests (GPU)", "E2E tests (Strix Halo, Ubuntu)", "E2E tests (Strix Halo, Windows)" and "E2E consolidated report", and a job-level if: skip reports as satisfied. So the PR-skip of the Strix lanes does not stall the queue.
• Canary expectation claim checked against expectations.toml:64-69: serve-default-engine-inference xfails only on lemonade+linux (EAI-7423), so ExpectPass on MI300X, exactly as the feature comment says.
• No stale call sites: resolve() has one non-test caller (tests/e2e.rs:829), updated. nightly.yml is independent of the paths-filter. No tag allowlist or linter to update.
• cargo test -p e2e-cucumber --lib passes locally (57/57).

Not verified

• ci.yml was not machine-parsed here (no YAML module in this environment); expressions and indentation were checked by hand and all needs.changes.outputs.* references are valid.
• The two unchecked boxes in your test plan — every required GPU check produced on this PR, and a scoped dispatch confirming the canary serves only the single scenario — need real CI runs and remain the right gate before merge.

Nice change overall: the filter comments explain the reasoning rather than just the rule, and the new unit tests cover canary-on/canary-off for both @requires-gpu and @serves-on-gpu paths.

Thanks for the thorough pass. Addressed in ffbbc03 (also rebased onto current main):

• 1 — stale README tag table: added @canary and @serves-on-gpu to the tag vocabulary in tests/e2e-cucumber/README.md, and corrected the now-inaccurate "each job runs the whole suite" text and job table to reflect PR canary mode (MI300X runs only @canary) and the two Strix lanes skipping on pull_request.
• 2 — nothing enforced "exactly one @canary": added a unit test that parses the real .feature files and asserts exactly one scenario carries @canary, so a later refactor dropping/renaming it fails a cheap test instead of silently emptying the pre-merge serve smoke.
• 5 — misleading skip reason from gate ordering: moved the canary-mode skip after the host-applicability checks (no-GPU / OS) in resolve(), so a scenario inapplicable for a hard reason reports that reason.
• 3 — merge_group not a gating backstop: kept the behaviour (deliberate trade for serial Strix hardware) but made the ci.yml comment honest — the Strix lanes are continue-on-error on merge_group too, so a regression surfaces at nightly rather than blocking the queue; noted the condition under which they'd graduate to a true gate.

Deferred, with reasoning:

• 4 — dash-crate exclusion / shared-process-state: left as-is; you verified it's structurally sound and compile risk is covered by heavy/rust. The residual shared-init risk is caught one stage later when merge_group forces serve=true. Low severity, no change.
• 6 — resolve() growing three trailing bools: agreed, but this is cross-PR (test(e2e): retry a cold-start chat request and report why it failed #155/ci: narrow the GPU serve matrix with a serve paths-filter (EAI-7746) #156/test(e2e): gate heavy GPU serves to the merge queue #157 each add one). A shared RunMode-style struct is the right fix and should land once, in one place, rather than each PR reshaping the signature — better agreed separately than unilaterally here.

The two test-plan boxes (every required GPU check produced; scoped dispatch confirming the canary serves only the one scenario) are validated by the CI run on this push.

Re-review of ffbbc03e. Approve on the substance — the risky parts (coverage regression, required-check starvation) were re-checked from scratch and hold up. Prior findings 1, 2, 3 and 5 are fixed; 4 and 6 were deferred with reasoning I agree with, and 4 I independently re-verified (the dash crates really are unreachable from the serve path).

One blocker, and it is cross-PR rather than internal to this change.

Blocking: this PR and #157 make opposite decisions about the same scenario

Both add a 6th positional bool to resolve() in the same slot (canary_mode here, include_merge_queue there) — that alone is just rebase work. The semantic clash is the problem:

• This PR tags serve-default-engine-inference (6b) @canary — the only scenario that serves on a PR.
• test(e2e): gate heavy GPU serves to the merge queue #157 tags that same scenario @merge-queue, i.e. explicitly not on a PR, and designates 5 (vLLM) and 7 (lemonade) as the per-PR canaries instead.

If both land, 6b carries both tags. #157's merge_queue check sits before the canary check in resolve(), so on a PR the Skip fires first and the canary lane serves nothing — zero real serves per PR, silently, green in about two minutes. Neither PR's guard catches it: the cardinality test counts @canary tags, it never asserts the tagged scenario resolves to ExpectPass under canary mode.

Worth settling on one mechanism before either merges. #157's per-engine pair is strictly better coverage — vLLM and lemonade each get a real per-PR smoke, where a single canary only exercises whichever engine is the platform default. If this one lands first, #157 becomes a retag delta. That also disposes of prior finding 6: one flag instead of three trailing bools.

Non-blocking

• docs/ci-hardware-testing.md is now stale — same class as the README fix, missed at the repo root (and it's linked from ci.yml:677). Three statements are false: :20-21 "no separate tier flag or tag filter to maintain" (E2E_PR_CANARY is a tier flag, @canary/@serves-on-gpu are a tag set with a test enforcing cardinality); :46-51 "GPU jobs run ... on pull_request ... when the heavy path filter is true" (gating is on serve, and the Strix lanes no longer run on pull_request at all); :54 heavy → serve.
• No drift-guard on the serve crate list. heavy uses '**/*.rs'; serve is an explicit list, so a new serve-path crate silently drops off GPU coverage on PRs. The @canary tag got a cheap parser-based guard in this PR — the other half of the same gate could use one: assert the filter covers the apps/rocm/apps/rocmd dep closure. Mitigated by serve being forced true off-PR, so drift delays coverage rather than removing it.
• Stale comment at ci.yml:753-761 — "One job runs every applicable scenario (tiers collapsed)" no longer holds on the PR path.
• scripts/** is missing from the filter list in the description (it's at ci.yml:150).
• Both test-plan boxes look closed by run 30910513724 — all four required GPU contexts produced (GPU success, both Strix lanes skipped rather than pending, report success), and the e2e-gpu job ran with E2E_PR_CANARY: 1 for 27 scenarios with a single ~33s gap at 6b and everything else near-instant, consistent with exactly one real serve.

Tradeoff worth putting in the description

The body says "the full matrix runs in the merge queue", which reads as a backstop; the ci.yml comment is now honest that it isn't, but the body isn't, and the softening reaches further than the Strix lanes. Non-canary MI300X scenarios — serve-vllm-inference, serve-lemonade-inference, serve-readiness-contract, serve-vllm-default-on-instinct, short-name expansion/consistency, and the GPU-masking refusal paths — now have no blocking signal at all, falling back to nightly at day granularity, decoupled from the PR that broke them. That may well be the right trade for serial hardware; it should just be visible. Relatedly, the "proving-out" framing for continue-on-error is well evidenced by the flake-fix trail, but has no graduation criteria — worth a concrete bar, or it becomes permanent by default.

Checked and clean

serve filter completeness against the rebuilt apps/rocm/apps/rocmd dep closure (all globs correctly use /**); @serves-on-gpu tag set — every scenario across the 8 feature files traced into its step definitions, exactly four real-serve-without-@requires-gpu scenarios exist and all four are tagged; canary is meaningful (6b is ExpectPass on MI300X); no required-check starvation, including e2e-report's always() bypassing the implicit success() on skipped needs; heavy || serve is load-bearing (model_catalog.json matches serve but no heavy glob); E2E_PR_CANARY parsing is strict and consistent with its siblings.

The windows-build-and-test failure is unrelated — lifecycle-windows-http-install hits the loopback transport flake in #173, fixed by #174.

Nice touches: the filter comments explain the reasoning rather than restating the rule, including why root-only Cargo.toml beats **/Cargo.toml; moving the canary gate after the hard-applicability checks so a scenario reports its real skip reason; and the cardinality test is a good cheap pattern that fails loudly on tag drift.

Thanks @juhovainio — you're right about the operator-precedence bug in the Strix if: conditions (&& binding tighter than ||, so the merge_group branch bypassed the build/serve checks). Good catch.

That gating lived in ci.yml in the previous commits. Since this PR was opened, #193 (EAI-7548) split the self-hosted GPU lanes into their own e2e-selfhosted.yml, so those ci.yml GPU jobs no longer exist. I've force-pushed a re-targeted version (now a single commit) that:

• applies the serve paths-filter to e2e-selfhosted.yml's changes job (where the GPU lanes now live) and repoints its three GPU jobs + the consolidated-report gate from heavy to serve;
• drops the earlier canary mechanism entirely (no @canary/@serves-on-gpu, no resolve() bool) — this also removes the buggy Strix if: blocks you flagged, so the precedence issue is gone rather than patched;
• keeps the GPU-job if: conditions in their original, correctly-grouped form ((github.event_name != 'workflow_dispatch' && needs.changes.outputs.serve == 'true') || (dispatch ...)) — no unparenthesized merge_group || push && ... construct anywhere in the new file.

Net diff is now just e2e-selfhosted.yml + docs/ci-hardware-testing.md. The workflow_contract tests pass, and the container gate is green apart from one unrelated pre-existing failure (extracting_the_sdk_archive_removes_it, from #166 — the gate image lacks tar; green on GitHub's Linux runners).

Thanks — you read it right, and the answer is "trim the description": no commits are missing.

The canary / merge-queue-gating work was deliberately dropped from this PR. It collided with #157, which tags the same scenario (serve-default-engine-inference, 6b) @merge-queue where this PR had tagged it @canary; since merge_queue is checked before canary in resolve(), landing both would have left the per-PR canary lane serving nothing at all. #157's per-engine pair (5 vLLM + 7 lemonade) is strictly better coverage than a single canary, so this PR was reduced to just the orthogonal half — the paths-filter — and #157 kept the canary mechanism. #157 has since merged, so that half is already on main.

The description has now been updated to describe only what actually ships here, and the two stale Test Plan checkboxes are gone.

On the report gate: plain serve rather than heavy || serve is intentional in this workflow. Every job e2e-report consolidates in e2e-selfhosted.yml is itself serve-gated, so serve alone is the correct condition — the heavy || serve form belonged to the earlier ci.yml revision, where the report also had to cover the heavy-gated mock lane that lives in that workflow.

And thanks for double-checking the rocm-dash-* exclusion and the root-only Cargo.toml scoping — those were the two calls I most wanted a second pair of eyes on.